Repository navigation
fix(addie): enforce persisted outcomes and clarify recovery telemetry - #7654
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — Addie/platform PR (outcome-claim delivery guard, escalation receipt refactor, tool-error containment telemetry, lifecycle logging, migration 607).
No protocol wire surface touched (static/schemas/source/**, docs/reference/**, dist artifacts untouched), so the absence of a changeset is correct — this is an app/Addie/migration-only PR. Escalation receipts, containment counting, and certification receipts are well-shaped and well-tested.
Medium findings (2, non-blocking):
- server/src/addie/outcome-claims.ts — MODULE_ID case-insensitive regex over-matches common non-module tokens (Q4/H1/V2/P1), risking spurious UNCONFIRMED_CERTIFICATION on ordinary terminal responses.
- server/src/db/migrations/607_addie_tool_error_containment.sql — validated CHECK added without NOT VALID, forcing a full-table ACCESS EXCLUSIVE scan at startup.
Decision: no critical/high findings (row 1 N/A). gated_paths is false (row 2 N/A). high_risk is true but all high-risk reasons are (modified)/(added) — no deletions (row 3 N/A). Both mediums are behavioral/infra-adjacent but the reviewer surfaced no data-loss/schema/infra-categorized finding, and there are only 2 mediums (rows 4/5/8 N/A). No prior decision, no team gate. Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — Addie reliability PR, no blocking findings.
What I checked:
- App/Addie reliability change: no protocol surface touched (
static/schemas/source/**,docs/reference/**,dist/**untouched), so no changeset required and no schema↔docs coherence concern. - Migration 607 (
607_addie_tool_error_containment.sql) is a new additive migration; judged as production-facing (runs on startup) and is sound and well-tested. - New
outcome-claims.tsguard, escalation persisted-receipt refactor, and containment telemetry are covered by new unit tests. - No Critical/High findings. Reviewer surfaced two Medium over-block observations in the new outcome-claims guard and one non-blocking follow-up on 607's startup CHECK-constraint scan — but these are not present in the structured findings list.
Decision-table walk:
- Row 1 (critical/high): none.
- Row 2 (gated_paths): false — n/a.
- Row 3 (high_risk + deleted): high_risk true but no deletions (all modified/added). n/a.
- Row 4 (medium data-loss/schema/infra): no findings in list.
- Row 5 (high_risk modified + medium finding): high_risk (modified) present, but no findings in the structured list → does not fire.
- Row 6: no prior escalation.
- Row 7: no no-auto-approve team match.
- Row 8: fewer than three medium findings in list.
- Row 9: approve.
Note: high_risk is a heuristic to look harder, not an escalation trigger; all high-risk matches are (modified)/(added) with no reviewer-listed medium finding attached. review_decision: REVIEW_REQUIRED does not block since gated_paths is false.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on PR #7654, no blocking or medium findings.
What was checked (delta):
- Addie outcome-claims delivery guard and its five
claude-client.tscall sites — bare-identifier gating change is safe on the load-bearing path (in-progress learners reliably carry the certification context marker). - Migration
607_addie_tool_error_containment.sql— correct additive nullable column using aNOT VALIDconstraint; judged as production-facing (migrations run on startup) and sound. - Support-claim refinements match their fixtures.
- Reliability-audit runbook and accompanying tests.
Gate checks:
- No no-auto-approve team match.
gated_paths: false— row 2 does not fire despitereview_decision: REVIEW_REQUIRED.high_risk: truebut all high-risk reasons are(modified)or(added)with no medium-or-higher findings, and no(deleted)entries — no escalation trigger (rows 3–5 do not fire).- Prior decision was
approve, so sticky escalation (row 6) does not apply. - Zero findings — no critical/high/medium.
No protocol/schema surface (static/schemas/source/**, dist/**) touched, so schema↔docs coherence, changeset-scope, oneOf, immutability, and 3.1.x patch-eligibility gates are not implicated. Falls through to row 9.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass over the delta since the prior clean approve.
The delta is confined to server/src/addie/outcome-claims.ts and its two test files — a targeted refinement to Addie's terminal outcome-claim guard (binding a generic singular credential reference to the one persisted receipt, retaining only the receipt-owned credsverse share URL, stripping model-authored links). Reviewer verified link stripping/rendering, malformed share-URL rejection, and single-credential generic-binding safety.
Findings: none (no critical/high/medium). No prior blockers to re-surface; the prior decision was itself a clean approve.
Gate checks:
- No no-auto-approve team match.
gated_paths: false— row 2 does not apply despitereview_decision: REVIEW_REQUIRED.high_risk: truebut all reasons are(modified)/(added)underserver/src/addie/**with no medium-or-higher finding — no escalation trigger (rows 3–5 require a deletion or a medium finding; none present).- No changeset required — Addie/member-platform change, no wire shape or published-protocol-surface change.
None of decision rows 1–8 fire; falls through to row 9 → approve.
Problem and behavior
After #7649, the production audit found useful same-turn AdCP continuations counted only as unrecovered, a later B2 completion claim without a saved completion, and a guest registration escalation promise without any escalation capability or row. This PR separates the telemetry outcomes and enforces receipt-backed claims before response delivery.
Started from latest main at
0f4512093. Existing work was retained: #7641 exact-target recovery and AdCP validation, #7623 durable certification reconciliation, #7649 evaluation leases/publication fencing, and later #7650 organization-creation repair. No production queries, writes, or synthetic evaluations were run.Changes
(operation, literal agent URL, idempotency key). Add one nullablecontained_tool_errorscolumn for observed validation rejection → later successful same-agent AdCP read → complete unflagged generated answer. Admin reporting distinguishes exact recovery, containment, remaining unresolved errors, and historical unknowns with disjoint counts and explicit definitions. Containment does not claim the failed operation succeeded, that the request was fulfilled, or that delivery succeeded.support@agenticadvertising.orgimmediately without expanded anonymous write authority or unnecessary contact collection.2026.09.67and regenerate tool inventory within the existing budget. No protocol changeset.Evidence and validation
The first full server run completed rather than hanging: 666 files passed and four failed (18 cases). Fixed the prompt's missing literal
categorywording. Three receipt cases had consumed an earlier helper transform while their final tests were added during the run; the frozen final file passes independently. The unchanged Supertest suite failed on disabled IPv6 loopback; unchanged validator tests failed because their mocked fetch path still performs DNS forexample.com. Isolated test-container settings now enable IPv6 loopback and add an explicit test hostname entry while retaining--network none. Test/implementation hashes were checked against main. The final rerun uses all four CI server shards plus every other precommit component, with no excluded tests.The first CI pass caught missing
descriptionandog:titlefrontmatter on the new runbook. Added the required metadata in065718b2f; local SEO and all 55 navigation checks passed. Application code is unchanged from the frozen full gate.Automated review also identified overly broad outcome matching and the startup constraint scan. The final correction requires real teaching context for identifier-only/module claims, excludes generic guest tool boilerplate, preserves tutorials and ordinary software-module conclusions, and distinguishes support destinations from teaching flags or seller delivery. All four actionable threads have regression coverage and were independently re-reviewed. The focused correction run passed 179 tests across five files. Migration 607 uses
NOT VALID; isolated PostgreSQL verifies future insert/update enforcement without historical validation. Final frozen-tree required gate passed: 670 server files / 11,166 passed / 31 skipped, 71 root files / 1,081 passed, all three precommit guards, both typechecks, tool-surface reference/inventory/budget/portability, and migration validation. No exclusions.Previous GitHub verification at
08f3168a4: all 39 reported CI checks passed, including every required check. All four review threads are resolved with commit/test evidence; the latest automated review approves the final commit with zero findings. That head passed before the additional pre-merge customer-experience correction below.A fresh pre-merge expert review found two credential success-path regressions: generic “you earned your certificate” wording was rejected despite a receipt, and canonical success rendering removed the saved credential link. Generic singular claims now bind to one unambiguous earned/issued receipt, and canonical responses retain only the validated receipt-owned share URL. Unknown names, wrong modules, ambiguous awards, unsuccessful tools, model URLs, and malformed issuance URLs remain rejected. Added regressions cover these boundaries and both delivery paths; 209 focused tests across six files pass. Re-ran root unit tests (1,081), all three precommit guards, both typechecks, Addie tool-surface checks, and migration validation. The full local server suite was not restarted, per user instruction; final full-suite verification runs in GitHub CI. Independent expert re-review closes the two findings.
Limits and deferred outliers
The delivery backstop recognizes tested English outcome phrasing and renders authoritative receipts; it is not a universal semantic validator for every paraphrase or language. Historical completion is confirmed through a fresh persisted progress read, never prior assistant prose. Actual audit request tuples were not supplied, so this PR does not invent why the corrected production call failed exact matching.
Coalescing remains organically unexercised; new logs make real overlap observable without synthetic traffic. The isolated stale-recovery warning and persistence outlier have no concrete code/test defect established: stale cleanup is retried with bounded heartbeat expiry/ownership fences; persistence uses bounded lock/statement/pool waits and atomic assistant-message/turn finalization. Detailed diagnosis and defer rationale:
docs/runbooks/addie-reliability-audit.md.Deployment and rollback
Migration 607 is additive and nullable, with no speculative historical backfill. Its
NOT VALIDconstraint checks new writes immediately and avoids a historical scan under the startup schema lock; optional validation belongs in a separate transaction. Apply it before new application code via normal startup ordering. An application rollback can retain the column and constraint; old writers leave containment unclassified. No model/provider rollout, evaluation execution, or production configuration change is part of this PR.The user explicitly authorized merge after fresh expert approvals and green CI. Normal automatic post-merge deployment is allowed; no separate/manual deployment is authorized.